Added insecureString options to allow to easier migration to secure setting variants#5496
Conversation
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #5496 +/- ##
============================================
- Coverage 70.85% 70.82% -0.03%
- Complexity 56553 58693 +2140
============================================
Files 4722 4769 +47
Lines 267594 280696 +13102
Branches 39212 40535 +1323
============================================
+ Hits 189600 198801 +9201
- Misses 61983 65563 +3580
- Partials 16011 16332 +321
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
dblock
left a comment
There was a problem hiding this comment.
Should we be deprecating insecure settings instead rather than just warn-usage?
|
We could put out the warning in 2.x line and probably deprecate it in 3.x? |
The issue discussed over in security plugin was related to not breaking existing setups and plugins that might still expect the insecure setting to be used. Just using |
dblock
left a comment
There was a problem hiding this comment.
Last question: are we going to flood logs with these? should the warning happen once per node/constructor?
I don't think the logs will be flooded, but any time the settings is queried it will log yes. Typically this only occurs at startup for a plugin, but I see the risk where a plugin might repeatedly read the setting (as say part of a custom mapping). The issue with construction time is that we don't yet know whether the insecure setting is actually used in configuration. It should be trivial to add a volatile or sync block around the current log statement and only log it once. |
dblock
left a comment
There was a problem hiding this comment.
Add a test that it's not logged twice in a separate test.
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Failing due to version bump after 2.4.1 release, #5560. Fixed is merged into main branch. @chriswhite199: Can you please rebase your branch against latest |
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
|
Looks like this has been ready to merge for a while but got missed out amongst our other PRs. Since the baseline is quite dated, i'll rebase from the current |
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Signed-off-by: Chris White <chriswhite199@gmail.com>
Gradle Check (Jenkins) Run Completed with:
|
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-5496-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0b775e79f637c7225ae908e368f1a62ce9ece011
# Push it to GitHub
git push --set-upstream origin backport/backport-5496-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.xThen, create a pull request where the |
…-project#5496) Signed-off-by: Chris White <chriswhite199@gmail.com> (cherry picked from commit 0b775e7)
…-project#5496) Signed-off-by: Chris White <chriswhite199@gmail.com>
…-project#5496) (opensearch-project#8094) Signed-off-by: Chris White <chriswhite199@gmail.com> (cherry picked from commit 0b775e7) Co-authored-by: Chris White <chriswhite199@gmail.com>
…-project#5496) Signed-off-by: Chris White <chriswhite199@gmail.com>
…-project#5496) Signed-off-by: Chris White <chriswhite199@gmail.com> Signed-off-by: Shivansh Arora <hishiv@amazon.com>
Description
To address discussion points in the following issues and PRs in common-utils and security plugin:
This change adds override options to SecureSetting.insecureString such that migrating from legacy insecure settings to secure variants can be achieved without breaking existing cluster deployments and having to set
opensearch.allow_insecure_settings. Instead usage of deprecated insecure settings are logged (warn level) with a note as to the new secure variant of the setting too.If approved, changes can be made to common-utils and security plugin to utilize this new code